Skip to content

Fix time database update calculations #17735

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Sep 12, 2025
Merged

Fix time database update calculations #17735

merged 2 commits into from
Sep 12, 2025
+81 −3

Conversation

pcd1193182
Copy link
Contributor

Sponsored by: [Klara, Inc.; Wasabi Technology, Inc.]

Motivation and Context

The time database update math assumed that the timestamps were in nanoseconds, but at some point in the development or review process they changed to seconds. This resulted in no entries ever entering the day or month databases, or at least not for another 2.7 million years or so (24 * 60 * 60 * 10^9 seconds).

Description

This PR fixes the math to use seconds instead.

How Has This Been Tested?

Manually verified with a test pool and an increased update frequency

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Quality assurance (non-breaking change which makes the code more robust against bugs)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Sep 12, 2025
@behlendorf
Copy link
Contributor

Manually verified with a test pool and an increased update frequency

It would be good to have some kind of simulated test case for this. Clearly we've got a significant gap here in the testing.

@pcd1193182
Copy link
Contributor Author

It would be good to have some kind of simulated test case for this. Clearly we've got a significant gap here in the testing.

So the current test uses the functionality to do scrubs, which is useful, but doesn't catch this issue. The way to test this would be to set the update frequency low, allow a few txgs to sync, and then verify that each of the minute day and month entries in the zap have non-zero values in them. It wouldn't be too hard to write; I can add that to the PR.

Paul Dagnelie added 2 commits September 12, 2025 12:50
Fix time database update calculations
02d82d9 
The time database update math assumed that the timestamps were in
nanoseconds, but at some point in the development or review process they
changed to seconds. This PR fixes the math to use seconds instead.

Signed-off-by: Paul Dagnelie <paul.dagnelie@klarasystems.com>
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
add test for database updates
f78eae6 
Signed-off-by: Paul Dagnelie <paul.dagnelie@klarasystems.com>
@github-actions github-actions bot removed the Status: Accepted Ready to integrate (reviewed, tested) label Sep 12, 2025
@amotin
Copy link
Member

amotin commented Sep 12, 2025

The test does not really test that intervals are right, just that they are smaller then time from epoch. But since I suppose it would fail without this fix, it is better then nothing.

@amotin amotin added the Status: Accepted Ready to integrate (reviewed, tested) label Sep 12, 2025
@pcd1193182
Copy link
Contributor Author

The test does not really test that intervals are right, just that they are smaller then time from epoch. But since I suppose it would fail without this fix, it is better then nothing.

Agreed; it would be nice to be able to verify that, but setting the time on the system turns out to be annoying to do portably.

@behlendorf behlendorf merged commit 9b772f3 into openzfs:master Sep 12, 2025
21 of 24 checks passed
pcd1193182 added a commit to pcd1193182/zfs that referenced this pull request Sep 12, 2025
@pcd1193182
Fix time database update calculations
94cf41f 
The time database update math assumed that the timestamps were in
nanoseconds, but at some point in the development or review process they
changed to seconds. This PR fixes the math to use seconds instead.
    
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alexander Motin <alexander.motin@TrueNAS.com>
Signed-off-by: Paul Dagnelie <paul.dagnelie@klarasystems.com>
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Closes openzfs#17735
pcd1193182 added a commit to pcd1193182/zfs that referenced this pull request Sep 13, 2025
@pcd1193182
Fix time database update calculations
f02cc6f 
The time database update math assumed that the timestamps were in
nanoseconds, but at some point in the development or review process they
changed to seconds. This PR fixes the math to use seconds instead.
    
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alexander Motin <alexander.motin@TrueNAS.com>
Signed-off-by: Paul Dagnelie <paul.dagnelie@klarasystems.com>
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Closes openzfs#17735
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Sep 15, 2025
@pcd1193182 @behlendorf
Fix time database update calculations
cac483d 
The time database update math assumed that the timestamps were in
nanoseconds, but at some point in the development or review process they
changed to seconds. This PR fixes the math to use seconds instead.
    
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alexander Motin <alexander.motin@TrueNAS.com>
Signed-off-by: Paul Dagnelie <paul.dagnelie@klarasystems.com>
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Closes openzfs#17735
pcd1193182 added a commit to pcd1193182/zfs that referenced this pull request Sep 16, 2025
@pcd1193182
Fix time database update calculations
9551e9d 
The time database update math assumed that the timestamps were in
nanoseconds, but at some point in the development or review process they
changed to seconds. This PR fixes the math to use seconds instead.
    
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alexander Motin <alexander.motin@TrueNAS.com>
Signed-off-by: Paul Dagnelie <paul.dagnelie@klarasystems.com>
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Closes openzfs#17735
trick2011 pushed a commit to trick2011/zfs that referenced this pull request Sep 16, 2025
@pcd1193182 @trick2011
Fix time database update calculations
d37e966 
The time database update math assumed that the timestamps were in
nanoseconds, but at some point in the development or review process they
changed to seconds. This PR fixes the math to use seconds instead.
    
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alexander Motin <alexander.motin@TrueNAS.com>
Signed-off-by: Paul Dagnelie <paul.dagnelie@klarasystems.com>
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Closes openzfs#17735
pcd1193182 added a commit to pcd1193182/zfs that referenced this pull request Sep 16, 2025
@pcd1193182
Fix time database update calculations
a37ca68 
The time database update math assumed that the timestamps were in
nanoseconds, but at some point in the development or review process they
changed to seconds. This PR fixes the math to use seconds instead.
    
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Alexander Motin <alexander.motin@TrueNAS.com>
Signed-off-by: Paul Dagnelie <paul.dagnelie@klarasystems.com>
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Closes openzfs#17735
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@behlendorf behlendorf behlendorf approved these changes

@amotin amotin amotin approved these changes

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pcd1193182 @behlendorf @amotin